Miscellaneous docstring additions and fixes #1998

Saransh-cpp · 2022-06-14T11:46:03Z

Added missing docstrings (in the manual), updated the existing ones, and added doctests in the following files -

stateless.jl
losses/functions.jl
utils.jl

In addition, there are various miscellaneous docstring fixes, and I have also prepended an _ to the internal API. This PR should close #1990.

This PR also closes #1993, but I am not 100% sure if the current implementation is wrong. I will revert the changes back if everything is correct.

PR Checklist

Tests are added
Entry in NEWS.md
Documentation, if applicable

codecov-commenter · 2022-06-14T15:50:00Z

Codecov Report

Merging #1998 (de72a12) into master (0b01b77) will not change coverage.
The diff coverage is 87.50%.

@@           Coverage Diff           @@
##           master    #1998   +/-   ##
=======================================
  Coverage   87.10%   87.10%           
=======================================
  Files          20       20           
  Lines        1528     1528           
=======================================
  Hits         1331     1331           
  Misses        197      197

Impacted Files	Coverage Δ
src/layers/stateless.jl	`100.00% <ø> (ø)`
src/losses/ctc.jl	`93.81% <ø> (ø)`
src/layers/normalise.jl	`88.81% <60.00%> (ø)`
src/utils.jl	`95.30% <90.00%> (ø)`
src/layers/basic.jl	`77.32% <100.00%> (ø)`
src/layers/conv.jl	`88.00% <100.00%> (ø)`
src/losses/functions.jl	`98.64% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b01b77...de72a12. Read the comment docs.

Saransh-cpp · 2022-07-26T18:54:01Z

cc: @DhairyaLGandhi

mcabbott · 2022-07-28T22:00:12Z

src/layers/normalise.jl

@@ -10,7 +10,7 @@ _dropout_shape(s, dims) = tuple((i ∉ dims ? 1 : si for (i, si) ∈ enumerate(s
 _dropout_kernel(y::T, p, q) where {T} = y > p ? T(1 / q) : T(0)

 """
-    dropout([rng = rng_from_array(x)], x, p; dims=:, active=true)
+    dropout([rng = _rng_from_array(x)], x, p; dims=:, active=true)


Maybe this shouldn't mention this internal function? If you aren't looking at the source, you don't need to know it exists:

Suggested change

dropout([rng = _rng_from_array(x)], x, p; dims=:, active=true)

dropout([rng], x, p; dims=:, active=true)

Alternatively, if it's a function you may need to know about & overload for your x, then it should be left as rng_from_array and included in the manual.

Interesting. The docstring does redirect users to the Dropout construct, so should we be concerned with users using this function?

If users are indeed using the dropout function, then it makes sense to treat rng_from_array as a public function and add it to the manual.

The majority of docstrings use the following notation for specifying the default value as rng_from_array-

[rng=GLOBAL_RNG]

Example -

kaiming_uniform([rng=GLOBAL_RNG], size...; gain = √2) -> Array kaiming_uniform([rng]; kw...) -> Function

glorot_normal([rng=GLOBAL_RNG], size...; gain = 1) -> Array glorot_normal([rng]; kw...) -> Function

etc.

I think these should also be changed. Now that I think of it, maybe we should keep it as rng_from_array and add it to the manual.

One point against mentioning rng_from_array in the docstring is that these functions don't actually use it! For most cases users will encounter it will behave as if it were, but there are enough edge cases that I'd prefer not to unintentionally mislead about expected behaviour.

Ah, so should I keep it private, and update the docstring as [rng]?

I stand corrected, it seems the functions in question were updated very recently to use it. Don't see much hurt in documenting rng_from_array and just noting it's an internal function, but you may want to wait for others to chime in. Another idea would be to call the zero-arg rng_from_array something else and document that. The current name is a bit of a misnomer anyways because there is no array involved in that method...

Oh yes, something like default_rng_value()? Then maybe add both of them to the manual?

src/layers/normalise.jl

mcabbott · 2022-07-28T22:02:42Z

src/utils.jl


-Return an `Array{Float32}` of the given `size`, filled like `rand` or `randn`.
+Return an `Array{Float32}` of the given `size`, filled like `rand`.
 When the size is not provided, `rand32(rng::AbstractRNG)` returns a function.
 """
 rand32(dims::Integer...) = Base.rand(Float32, dims...)


Should this also call _rng_from_array()?

Should _rng_from_array() be added in the following way? -

rand32(dims::Integer...) = Base.rand(_rng_from_array(), Float32, dims...)

Saransh-cpp · 2022-08-06T13:53:56Z

I have renamed rng_from_array() (with no arguments) to default_rng_value(), and added both of them to the manual.

ToucheSir · 2022-08-15T23:18:00Z

test/losses.jl

-  @test Flux.tversky_loss(ŷ, y) ≈ -0.06772009029345383
-  @test Flux.tversky_loss(ŷ, y, β=0.8) ≈ -0.09490740740740744
+  @test Flux.tversky_loss(ŷ, y) ≈ 0.028747433264887046
+  @test Flux.tversky_loss(ŷ, y, β=0.8) ≈ 0.050200803212851364


How did this change come about? It seems very drastic given the inputs have not changed.

I have tweaked the implementation here - https://github.com/FluxML/Flux.jl/pull/1998/files#diff-0ff18e54a53b74bc46260164231d6265a35569aa9a367cedda9f8baee211df42R534-R538.

See #1993. I'll revert the changes if the original implementation is correct. (Should have dealt with this in a separate PR, my bad.)

Yes, it would be best to revert for this PR. I'll follow up in #1993.

ToucheSir · 2022-08-17T22:54:37Z

Thanks @Saransh-cpp !

Saransh-cpp marked this pull request as draft June 14, 2022 11:51

Saransh-cpp marked this pull request as ready for review June 14, 2022 15:08

Saransh-cpp force-pushed the docstrings-for-utils-and-losses branch from de72a12 to a0a8536 Compare July 28, 2022 13:37

mcabbott reviewed Jul 28, 2022

View reviewed changes

mcabbott added the documentation label Jul 28, 2022

Saransh-cpp force-pushed the docstrings-for-utils-and-losses branch from 5c8b0a8 to 91013ff Compare August 6, 2022 09:08

Saransh-cpp requested review from ToucheSir and mcabbott August 15, 2022 19:31

ToucheSir reviewed Aug 15, 2022

View reviewed changes

Saransh-cpp added 9 commits August 16, 2022 20:28

Add doctests for losses and stateless layers

898b558

Fix and update docs for utils.jl

0a7f42a

Minor typos

0c7bdb9

Fix the implementation and docstring of tversky_loss

dcbdf07

Clean up the doctests + fix tests

02c8a95

Fix doctests

9dec787

rng_from_array() -> default_rng_value()

4773a69

fix tests (typos)

a67400f

Revert tversky_loss changes

f49ec34

Saransh-cpp force-pushed the docstrings-for-utils-and-losses branch from d03aed3 to f49ec34 Compare August 16, 2022 15:03

ToucheSir approved these changes Aug 17, 2022

View reviewed changes

ToucheSir merged commit d4f1d81 into FluxML:master Aug 17, 2022

ToucheSir mentioned this pull request Aug 25, 2022

Make create_bias a public API? #2049

Closed

mcabbott mentioned this pull request Jan 8, 2023

Move dropout to NNlib #2150

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Miscellaneous docstring additions and fixes #1998

Miscellaneous docstring additions and fixes #1998

Saransh-cpp commented Jun 14, 2022

codecov-commenter commented Jun 14, 2022

Saransh-cpp commented Jul 26, 2022

mcabbott Jul 28, 2022

Saransh-cpp Jul 29, 2022

Saransh-cpp Jul 29, 2022

ToucheSir Jul 30, 2022

Saransh-cpp Aug 1, 2022

ToucheSir Aug 1, 2022

Saransh-cpp Aug 1, 2022

mcabbott Jul 28, 2022

Saransh-cpp Aug 1, 2022

Saransh-cpp commented Aug 6, 2022

ToucheSir Aug 15, 2022

Saransh-cpp Aug 16, 2022

ToucheSir Aug 16, 2022

Saransh-cpp Aug 16, 2022

ToucheSir commented Aug 17, 2022

	dropout([rng = _rng_from_array(x)], x, p; dims=:, active=true)
	dropout([rng], x, p; dims=:, active=true)

Miscellaneous docstring additions and fixes #1998

Miscellaneous docstring additions and fixes #1998

Conversation

Saransh-cpp commented Jun 14, 2022

PR Checklist

codecov-commenter commented Jun 14, 2022

Codecov Report

Saransh-cpp commented Jul 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Saransh-cpp commented Aug 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ToucheSir commented Aug 17, 2022